cleanup: replace dial with newclient #8602

vinothkumarr227 · 2025-09-23T13:31:25Z

RELEASE NOTES: None

…s-tests-newclient

codecov · 2025-09-23T13:35:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.11%. Comparing base (7235bb7) to head (f1c77ee).
⚠️ Report is 23 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8602      +/-   ##
==========================================
+ Coverage   81.86%   82.11%   +0.24%     
==========================================
  Files         415      415              
  Lines       40694    40711      +17     
==========================================
+ Hits        33316    33429     +113     
+ Misses       5993     5900      -93     
+ Partials     1385     1382       -3

see 40 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

clientconn_parsed_target_test.go

arjan-bal · 2025-09-25T08:38:57Z

clientconn_parsed_target_test.go

-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",


We should not remove these test cases. Since NewClient doesn't connect like Dial, we need to do the following to get an error instead:

Call cc.Connect()

Wait for TRANSIENT_FAILURE state using AwaitState. If using this function causes a circular dependency, copy it over to this file, adding a comment that mentioning the reason for the duplication.

If context times out before TF state, fail the test.

I am using Idle because these malformed targets result in zero resolved addresses. The client never attempts a connection, so it stays IDLE and cannot reach TRANSIENT_FAILURE

There's an INFO log similar to the following from the unix resolver:

INFO clientconn.go:333 [core] [Channel #5] Channel failed to exit idle mode: invalid (non-empty) authority: authority (t=+1.01919ms)

Since this is the only indication that the connection attempt failed (no errors are returned), I think this should be logged as an ERROR. Presently, the method responsible for channelz trace event logging is logging everything at as an INFO log. We could have at accept the severity as an additional param to achieve this. Once this is an error log, we can assert that the error is logged similar to the following.

grpc-go/internal/transport/keepalive_test.go

Line 401 in 8389ddb

grpctest.ExpectError("Client received GoAway with error code ENHANCE_YOUR_CALM and debug data equal to ASCII \"too_many_pings\"")

@dfawley do you think we should log the failure to exit idle mode in cc.Connect() as an ERROR instead of INFO.

Two options:

Change the unix name resolver not to error from Build when this happens. Instead, have it produce a ResolverError and otherwise do nothing.

Handle resolver.Build errors so that all name resolvers that error during Build will instead effectively produce a ResolverError. This fixes that class of problem in the same way for all name resolvers.

Either way, RPCs will fail with that error message text, and we shouldn't need anything besides INFO (or even V2?) logs.

clientconn_parsed_target_test.go

clientconn_test.go

default_dial_option_server_option_test.go

resolver_test.go

eshitachandwani · 2025-10-02T09:09:05Z

clientconn_parsed_target_test.go

+			ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+			defer cancel()
+			cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials()))
+			if err != nil {


We are changing the check condition here , is it intentional? If it is , the error message isn't correct. We are checking err!=nil which means NewClient failed and the error message says NewClient succeeded which is not right.

eshitachandwani · 2025-10-02T19:39:41Z

clientconn_parsed_target_test.go

 			}

-			cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()), WithContextDialer(dialer))
+			cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()), withDefaultScheme(defScheme), WithContextDialer(dialer))


I don't think we should override the default scheme to passthrough with new client because we want to check the behavior of parsed strings with defualt NewClient function with custom dialer. And the behavior might be different , because dns resolver also does its own parsing , so the results might be different. Correct me if I have understood incorrectly. But I think we should have a separate test for NewClient's default parsed address behavior with custom dialer

Thanks for your feedback, I used withDefaultScheme only in the specific tests where it’s needed, as suggested by Arjan. For checking the default NewClient behavior with a custom dialer, I’m not overriding the scheme. If you’d like a separate test specifically for NewClient's default behavior, let me know and I can add it.

eshitachandwani · 2025-10-02T19:40:41Z

clientconn_test.go

 	if err != nil {
 		t.Fatalf("Dial failed. Err: %v", err)
 	}
+	client.Connect()


Why did we add this here? We did not change from Dial to NewClient in the test and Dial will connect automatically?

No need to change, I reverted it back.

arjan-bal · 2025-10-06T08:03:58Z

clientconn_parsed_target_test.go

-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",


There's an INFO log similar to the following from the unix resolver:

INFO clientconn.go:333 [core] [Channel #5] Channel failed to exit idle mode: invalid (non-empty) authority: authority (t=+1.01919ms)

Since this is the only indication that the connection attempt failed (no errors are returned), I think this should be logged as an ERROR. Presently, the method responsible for channelz trace event logging is logging everything at as an INFO log. We could have at accept the severity as an additional param to achieve this. Once this is an error log, we can assert that the error is logged similar to the following.

grpc-go/internal/transport/keepalive_test.go

Line 401 in 8389ddb

grpctest.ExpectError("Client received GoAway with error code ENHANCE_YOUR_CALM and debug data equal to ASCII \"too_many_pings\"")

@dfawley do you think we should log the failure to exit idle mode in cc.Connect() as an ERROR instead of INFO.

arjan-bal · 2025-10-06T08:08:43Z

clientconn_parsed_target_test.go

 			}
+			defer cc.Close()
+			cc.Connect()
+			testutils.AwaitState(ctx, t, cc, connectivity.Idle)


This is pretty much a no-op since the channel starts in IDLE. A better check would be to assert testutils.AwaitState with a short context. We should wait for a reply on my other comment though, it may change the assertion.

Thanks for the feedback. I'm still a little unclear on what I should do for this test right now.

arjan-bal · 2025-10-06T08:35:41Z

resolver_test.go

 	}
 	cc.Connect()
-	if got, want := <-addrCh, target; got != want {
+	if got, want := <-addrCh, "caseTest2:///localhost:1234"; got != want {


Can we revert this change? It seems to be a no-op since the target variable has the same value.

vinothkumarr227 added 5 commits September 23, 2025 02:14

Updated NewClient instead of Dial

65f9bbe

Merge remote-tracking branch 'origin/master' into update-docs-example…

578671c

…s-tests-newclient

Fixed the test issues

c83de4b

small tweaks

714bd88

small tweaks

b060b0c

arjan-bal reviewed Sep 25, 2025

View reviewed changes

arjan-bal assigned vinothkumarr227 Sep 25, 2025

arjan-bal added the Type: Internal Cleanup Refactors, etc label Sep 25, 2025

arjan-bal added this to the 1.77 Release milestone Sep 25, 2025

vinothkumarr227 added 3 commits September 30, 2025 12:41

Fixed the review changes

98695b5

small tweaks

1a53f2b

small tweaks

889cf0c

eshitachandwani reviewed Oct 2, 2025

View reviewed changes

Fixed the review changes

66a1b3c

vinothkumarr227 removed their assignment Oct 6, 2025

arjan-bal mentioned this pull request Oct 6, 2025

Update docs and examples and tests to use NewClient instead of Dial #7049

Open

3 tasks

arjan-bal self-assigned this Oct 6, 2025

arjan-bal reviewed Oct 6, 2025

View reviewed changes

arjan-bal assigned dfawley and vinothkumarr227 and unassigned arjan-bal Oct 6, 2025

dfawley removed their assignment Oct 6, 2025

small tweaks

f1c77ee

easwars added the Status: Requires Reporter Clarification label Oct 7, 2025

vinothkumarr227 removed their assignment Oct 8, 2025

cleanup: replace dial with newclient #8602

Are you sure you want to change the base?

cleanup: replace dial with newclient #8602

Conversation

vinothkumarr227 commented Sep 23, 2025

Uh oh!

codecov bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 23, 2025 •

edited

Loading